Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update subcommand #140

Merged
merged 27 commits into from
Jun 13, 2023
Merged

Update subcommand #140

merged 27 commits into from
Jun 13, 2023

Conversation

u-train
Copy link
Contributor

@u-train u-train commented Jun 9, 2023

This adds an update subcommand per issue #4.

u-train added 9 commits June 8, 2023 22:57
This will allow us to test the update tool in all expected situations
- One dependency added/removed
- One dependency updated
- Transitive dependencies changed

It's also a nice addition for testing any other commands as well.
Metadata and zipped up
Copy link
Member

@magnalite magnalite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great start! To formalise a discussion from discord:

This should modify the manifest as well as the lockfile. We should also notify the user if there is a newer semver incompatible version of a package which we can't auto upgrade to. The thought process behind this is people will usually wally update blind and may be unaware there is a new major version of a package.

We should also add convenience flags to allow the user to specify if they want to upgrade everything to the latest possible version even if they are semver incompatible or if they do not want any semver incompatible choices. They might have many dependencies and saying yes or no is annoying!

src/commands/mod.rs Outdated Show resolved Hide resolved
src/commands/update.rs Outdated Show resolved Hide resolved
src/commands/update.rs Show resolved Hide resolved
src/commands/update.rs Outdated Show resolved Hide resolved
src/commands/update.rs Outdated Show resolved Hide resolved
src/commands/update.rs Outdated Show resolved Hide resolved
src/commands/update.rs Outdated Show resolved Hide resolved
test-projects/diamond-graph/root/0.1.0/wally.lock Outdated Show resolved Hide resolved
tests/integration/temp_project.rs Show resolved Hide resolved
@u-train
Copy link
Contributor Author

u-train commented Jun 11, 2023

This should modify the manifest as well as the lockfile. We should also notify the user if there is a newer semver incompatible version of a package which we can't auto upgrade to. The thought process behind this is people will usually wally update blind and may be unaware there is a new major version of a package.

We should also add convenience flags to allow the user to specify if they want to upgrade everything to the latest possible version even if they are semver incompatible or if they do not want any semver incompatible choices. They might have many dependencies and saying yes or no is annoying!

This should be postponed and done in a separate PR. It turns it, trying to do this gets complicated really fast.

  • You have to start dealing with interactive user input.
  • You have to deal with various edge cases based on the requirement in the manifest, what the lock file has if it does exist, and what versions are available upstream.
  • Yanking will only add more debt to solve in the future, too.
  • Then, trying to test.

I think it's better to keep the velocity going then to get this stuck here.

@u-train
Copy link
Contributor Author

u-train commented Jun 11, 2023

also the test cases are still broken :(

u-train added 4 commits June 11, 2023 21:05
They say to run your tests before push..
This is why.
Import in the wrong order...
Extra newline at the end...
@u-train u-train force-pushed the update-subcommand branch from a457ae5 to 839c713 Compare June 12, 2023 02:58
@u-train u-train marked this pull request as ready for review June 12, 2023 04:08
@magnalite
Copy link
Member

This should be postponed and done in a separate PR. It turns it, trying to do this gets complicated really fast.

  • You have to start dealing with interactive user input.
  • You have to deal with various edge cases based on the requirement in the manifest, what the lock file has if it does exist, and what versions are available upstream.
  • Yanking will only add more debt to solve in the future, too.
  • Then, trying to test.

I think it's better to keep the velocity going then to get this stuck here.

Ah yeah, I agree then. It is far more important that the command exists than for it to be ergonomic in all cases. If anyone really does want to upgrade everything to latest they can just yeet their lockfile and reinstall anyway.

Copy link
Member

@magnalite magnalite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking great! I believe it now works as expected and is tested enough to trust! I just have a couple nitpicks for you to check.

src/commands/update.rs Outdated Show resolved Hide resolved
src/commands/update.rs Outdated Show resolved Hide resolved
src/commands/update.rs Outdated Show resolved Hide resolved
src/commands/update.rs Outdated Show resolved Hide resolved
src/commands/update.rs Outdated Show resolved Hide resolved
u-train and others added 3 commits June 12, 2023 19:37
Seemingly forgot to filter out packages to update for try_to_use.
instead it kept them and got rid of everything else.
Copy link
Member

@magnalite magnalite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing work getting this polished up!

@magnalite magnalite merged commit 84a81e8 into UpliftGames:main Jun 13, 2023
@magnalite magnalite mentioned this pull request Jun 13, 2023
@magnalite magnalite added this to the Essential feature complete milestone Jun 13, 2023
MobiusCraftFlip pushed a commit to MobiusCraftFlip/wally that referenced this pull request Oct 15, 2023
* Refactored TempProject into its own module and started on tests

* Created test projects with a diamond-graph
This will allow us to test the update tool in all expected situations
- One dependency added/removed
- One dependency updated
- Transitive dependencies changed

It's also a nice addition for testing any other commands as well.

* Added to primary-registry
Metadata and zipped up

* Built the update subcommand.

* Lockfiles need to be written!

* Fixed misspelling of dependency_changes

* Reverted test_registry arg back to invisible

* Changed target_packages to package_specs
Clarity?

* Downgrading was added as a concept

* Added lockfile method to view packages as packageIds

* Installs the new packages now, forgor.

* No longer throws if missing lockfile
It will generate a new one instead based on the
manifest.

* Clarity on the filtering process for try_to_use

* Added needed imports
-- they got stashed by accident

* Made the clippy happy and fixed grammar mistakes

* Delete root from registry
They're not packages to be consumed.

* Add new tests, almost done with them all so far.

* Added the rest of snapshots missing

* Testing now works, yay!

* Deleted testing code by mistake and forgot snap
They say to run your tests before push..
This is why.

* Appleasing the formatter
Import in the wrong order...
Extra newline at the end...

* Made it look... *pretty* and cleaned messes

* another blood sarciface for rust fmt

* Doing final cleanups per comments upstream

* The gods demand it, another formatter sacrifice

Co-authored-by: magnalite <[email protected]>

* The coolness must be toned down.

* A little silly mistake indeed
Seemingly forgot to filter out packages to update for try_to_use.
instead it kept them and got rid of everything else.

---------

Co-authored-by: magnalite <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants